@jlblancoc

glad to see you working on this! Had some questions about this though.

Shouldn't https://github.com/borglab/gtsam/blob/develop/gtsam/inference/Ordering.cpp#L28
be changed to just <metis.h>
as well as https://github.com/borglab/gtsam/blob/develop/gtsam_unstable/partition/FindSeparator-inl.h#L25 being changed to <metislib.h> in this PR?

Good catch! 👍

based on my past effort on this task, I fear you might not be able to use <metislib.h>. Re: #292 (comment)

We should create an issue for @acxz's comments and tackle them in the next PR. This PR is atomically very nice.

I don't want to be a downer but I don't think this PR should be merged before addressing the issues I have raised.
For one we are still using the older include path which contains the 3rdparty path. This means that even if I am setting the SYSTEM_METIS flag to be on, I still won't be using the metis system include file.
Even if that is changed, system metis does not provide metislib.h which means we won't be able to use the functionality of it. Thus rendering the SYSTEM_METIS flag broken. If its going to be broken when merged in, we should just not merge it in.

As for an issue, my concerns and past efforts are documented in that above linked comment which is on the use system libraries issue thread anyway. I think that is good enough for now.
Although I guess creating an issue for "Remove internal metislib api calls from GTSAM code" would be warranted.

Fair point. I'm going to leave this as approved and trust @jlblancoc to add in the updates before we merge.

@jlblancoc @acxz I added #ifndef guards if we're using the system metis. This should allow us to use either metis without issue.

How can I actually compile and run the tests that have the above #ifndefs?
I tried naively by running make all.tests, but I get this compilation error:

In file included from /home/pfrommer/Documents/gtsam/CppUnitLite/TestHarness.h:23,
                 from /home/pfrommer/Documents/gtsam/gtsam/navigation/tests/testMagFactor.cpp:25:
/home/pfrommer/Documents/gtsam/gtsam/navigation/tests/testMagFactor.cpp: In member function ‘virtual void MagFactorunrotateTest::run(TestResult&)’:
/home/pfrommer/Documents/gtsam/gtsam/navigation/tests/testMagFactor.cpp:67:40: error: reference to ‘_1’ is ambiguous
   67 |       (std::bind(&MagFactor::unrotate, _1, nM, none), theta), H, 1e-6));
      |                                        ^~
/home/pfrommer/Documents/gtsam/CppUnitLite/Test.h:152:9: note: in definition of macro ‘EXPECT’
  152 | { if (!(condition)) \
      |         ^~~~~~~~~
In file included from /home/pfrommer/Documents/gtsam/gtsam/3rdparty/Eigen/Eigen/Core:281,
                 from /home/pfrommer/Documents/gtsam/gtsam/3rdparty/Eigen/Eigen/Dense:1,
                 from /home/pfrommer/Documents/gtsam/gtsam/base/OptionalJacobian.h:22,
                 from /home/pfrommer/Documents/gtsam/gtsam/base/Matrix.h:27,
                 from /home/pfrommer/Documents/gtsam/gtsam/base/Manifold.h:22,
                 from /home/pfrommer/Documents/gtsam/gtsam/base/GenericValue.h:22,
                 from /home/pfrommer/Documents/gtsam/gtsam/nonlinear/Values.h:27,
                 from /home/pfrommer/Documents/gtsam/gtsam/nonlinear/NonlinearFactor.h:23,
                 from /home/pfrommer/Documents/gtsam/gtsam/navigation/MagFactor.h:19,
                 from /home/pfrommer/Documents/gtsam/gtsam/navigation/tests/testMagFactor.cpp:19:
/usr/include/c++/9/functional:211:34: note: candidates are: ‘const std::_Placeholder<1> std::placeholders::_1’
  211 |     extern const _Placeholder<1> _1;

self-answer: to build the test, do:

make -testFindSeparator.run

Looks like we are still missing to change https://github.com/borglab/gtsam/blob/develop/gtsam/inference/Ordering.cpp#L28
to something like:

#ifdef GTSAM_SUPPORT_NESTED_DISSECTION
#ifdef GTSAM_USE_SYSTEM_METIS
#include <metis.h>
#else
#include <gtsam/3rdparty/metis/include/metis.h>
#endif
#endif

see @acxz comment above

@berndpfrommer made the changes. Can you please review?

Looks good to me.
I compiled and did make testFindSeparator.run both with and without using the metis system library, and both scenarios worked as they should (using system libraries the test didn't run, as expected).

I also added a CI case for using only system libraries. Currently it only sets the Eigen and Metis system library flags, but we can update those as we go.

Let's merge? @jlblancoc please do the honors.

@jlblancoc I'm merging this. 🙂

Let's merge? @jlblancoc please do the honors.

@varunagrawal Sorry, I've been busy and missed a lot of what's happening here... so glad you guys made it work :-)